resolve subgraph staging sequences via regclass#2446
Conversation
The vertex/edge staging copies in create_subgraph() generated new
graphids with nextval(%L), which binds the sequence as a string literal
and invokes the nextval(text) overload. That re-resolves the
schema-qualified sequence name on each call.
Cast the literal to regclass (nextval(%L::regclass)) so the sequence is
resolved once to its OID, matching how AGE defines its label id defaults
(nextval('schema.seq'::regclass)). Applied to both the vertex and edge
staging queries, in sql/age_subgraph.sql and the identical body in the
age--1.7.0--y.y.y.sql upgrade template so the upgrade-path catalog
comparison still matches.
Behavior is unchanged; all 38 regression tests pass against PostgreSQL 18.
Addresses Copilot review feedback on apache#2441.
Co-authored-by: GitHub Copilot (Claude Opus 4.8) <[email protected]>
modified: age--1.7.0--y.y.y.sql
modified: sql/age_subgraph.sql
There was a problem hiding this comment.
Pull request overview
Updates the dynamic SQL in ag_catalog.create_subgraph() to call nextval() with a regclass-typed sequence reference, avoiding repeated schema-qualified sequence name resolution during vertex/edge staging while keeping behavior consistent with AGE’s existing sequence default patterns.
Changes:
- Use
nextval(%L::regclass)(instead ofnextval(%L)) for vertex staging ID generation. - Use
nextval(%L::regclass)(instead ofnextval(%L)) for edge staging ID generation. - Apply the same change to both the canonical SQL and the extension upgrade template to keep upgrade-path objects identical.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| sql/age_subgraph.sql | Casts formatted sequence literal to regclass in vertex/edge staging nextval() calls. |
| age--1.7.0--y.y.y.sql | Mirrors the same regclass casting change in the upgrade template for catalog match parity. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
gregfelice
left a comment
gregfelice
left a comment
There was a problem hiding this comment.
LGTM — approving.
Confirmed the change is behavior-preserving: %L is fed dst_seq_fqn (the fully-qualified sequence name), so nextval(%L::regclass) yields nextval('schema.seq'::regclass), matching how AGE defines its label-id defaults. Both the old nextval(text) overload and '...'::regclass resolve the name to an OID through the same identifier-parsing path; the cast just resolves the constant once at plan time instead of re-resolving per row via the text overload. Same semantics, less per-row work.
Also verified the identical change is applied to both sql/age_subgraph.sql and the age--1.7.0--y.y.y.sql upgrade template, so the upgrade-path catalog comparison still matches. Existing regress/sql/subgraph.sql exercises create_subgraph, and the change is behavior-preserving, so no new test is needed.
The vertex/edge staging copies in create_subgraph() generated new graphids with nextval(%L), which binds the sequence as a string literal and invokes the nextval(text) overload. That re-resolves the schema-qualified sequence name on each call.
Cast the literal to regclass (nextval(%L::regclass)) so the sequence is resolved once to its OID, matching how AGE defines its label id defaults (nextval('schema.seq'::regclass)). Applied to both the vertex and edge staging queries, in sql/age_subgraph.sql and the identical body in the age--1.7.0--y.y.y.sql upgrade template so the upgrade-path catalog comparison still matches.
Behavior is unchanged; all 38 regression tests pass against PostgreSQL 18.
Addresses Copilot review feedback on #2441.
Co-authored-by: GitHub Copilot (Claude Opus 4.8) <[email protected]>
modified: age--1.7.0--y.y.y.sql
modified: sql/age_subgraph.sql